Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #2295 - Models with "@" in their name do not resolve as dependencies #3057

Merged
merged 1 commit into from
Aug 3, 2022

Conversation

bsorrentino
Copy link
Contributor

fix issue #2295

component: TypeScriptWorker (tsWorker.ts)

  • Enable 'skipEncoding' flag on Uri.toString invokation on getScriptFileNames() method
  • Compare 'fileName' argument provided to _getModel() method both with Uri encoded and not

component: TypeScriptWorker (tsWorker.ts)
- Enable 'skipEncoding' flag on Uri.toString invokation on 
getScriptFileNames() method
- Compare 'fileName' argument provided to _getModel() method both with 
Uri encoded and not
@hediet
Copy link
Member

hediet commented Apr 5, 2022

Thanks for the PR and your investigation!

I don't fully understand the problem though, why does this fix the issue?

@bsorrentino
Copy link
Contributor Author

bsorrentino commented Apr 5, 2022

Thanks for the PR and your investigation!

I don't fully understand the problem though, why does this fix the issue?

Hi @hediet thanks for starting review

I tackled issue #2295 integrating monaco editor in my app.

As you could see from issue's comments the problem was related to typescript language when we import the scoped package (@scope/package)

@lstkz has figure out that using %40 instead of @ the problem disappear ( e.g. using %40azure/msal instead of @azure/msal ) as states in this comment

With this hint I've investigated on the part that resolve the import and finally I got a fix.

Hope this clarify bit more

@hediet
Copy link
Member

hediet commented Apr 5, 2022

That I understand, by I don't understand the underlying issue.

I think the actual issue is that it is unclear how fileName should be encoded in private _getModel(fileName: string).

In this PR, I don't like the line if (uri.toString() === fileName || uri.toString(true) === fileName) { very much. Why do we need to check for both?

@hediet hediet added this to the April 2022 milestone Apr 5, 2022
@bsorrentino
Copy link
Contributor Author

bsorrentino commented Apr 5, 2022

That I understand, by I don't understand the underlying issue.

I think the actual issue is that it is unclear how fileName should be encoded in private _getModel(fileName: string).

In this PR, I don't like the line if (uri.toString() === fileName || uri.toString(true) === fileName) { very much. Why do we need to check for both?

Hi @hediet

The reason is that uri.toString() by default encode uri content itself. This for scoped packages cause that the @ char is converted to %40 and, in such case, the condition never match.

I've added the condition with uri.toString(true) in OR to be backward compatibile and allow matching condition

@pietrovismara
Copy link

Would be awesome to get this merged...

@peterp
Copy link

peterp commented May 15, 2022

@bsorrentino Throwing my two cents in here because this seems to be blocked.

I don't think the code clearly communicates why the two conditional checks are important, and your comment clarified the reasoning, but I get the sense that it would help to add that as a comment so that future readers of the code also understand why this was required.

Perhaps that may give @hediet or @alexdima a bit more confidence to merge this.

@bsorrentino
Copy link
Contributor Author

@bsorrentino Throwing my two cents in here because this seems to be blocked.

I don't think the code clearly communicates why the two conditional checks are important, and your comment clarified the reasoning, but I get the sense that it would help to add that as a comment so that future readers of the code also understand why this was required.

Perhaps that may give @hediet or @alexdima a bit more confidence to merge this.

Hi @peterp the suggestion here is to renew the PR adding meaningful comments to the affected code ?

@hrgdavor
Copy link

hrgdavor commented Jul 6, 2022

cmon. why is this PR still blocked ?

@hediet hediet modified the milestones: April 2022, July 2022 Aug 3, 2022
@hediet hediet linked an issue Aug 3, 2022 that may be closed by this pull request
Copy link
Member

@hediet hediet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some exploration, I also don't have a better idea than the change suggested in this PR.

@hediet hediet merged commit 913ae66 into microsoft:main Aug 3, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Sep 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Models with "@" in their name do not resolve as dependencies
6 participants